-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Go client libraries for etcd #11980
Conversation
17c68e4
to
51e6af2
Compare
I would like to kindly ask help on how to run the etcd related unit tests. So far, I have started etcd3 in docker according to instructions on etcd release notes, then executed |
Hi @tsaarni, I had issues running the docker container, so I followed the MacOS instructions in the link you provided instead. When I ran the test using the command you gave, I did see what looked like relevant traffic on port 2379:
There was lots of activity, but this packet seemed unambiguous:
The test also fails if I invoke it this way without etcd running. One thing to bear in mind is the I'll also warn you that we've had difficulties upgrading etcd in the past, involving some incompatibilities with it and gprc and cloud.google.com/go. So this may be a very nontrivial upgrade... |
This might be better now that etcd 3.5.0 is out. The problems we had were complicated but some notes I've consulted suggested that they might be resolved upon that release. Back to the testing: many of our backends are tested in CI using docker, e.g. https://github.com/hashicorp/vault/blob/main/physical/consul/consul_test.go#L159. Maybe you could do something similar as part of this PR? I don't know why we haven't already, but I suspect it's just that no one has gotten around to it yet. |
Hopefully that is the case. The version that is imported currently is bit problematic since some vulnerability scanners flag it as security issue.
Cool, yes I can add that. Thanks for your help! |
51e6af2
to
4807e31
Compare
I've added test container. I forced
I do not know the reason for this. It is running against etcd v3.5.0 server which I started with BTW the |
4807e31
to
003e0ef
Compare
Thanks for bringing this up. We discussed it internally and agree it makes sense to drop etcd v2 support. Would you like to tackle this as part of this PR? I'm happy to help however I can. |
Would it be OK to create another issue for removal of etcd2 support and continue with it in separate PR? I would be grateful if we could proceed with the client library upgrade separately from etcd2 deprecation, just to fix #9915 first. Would it be acceptable to proceed by disabling etcd2 tests in this PR (*)? (*) Meanwhile, I have figured out the reason for the etcd2 test case failures:
First, following observations
The root cause for the failure is the data model in etcd2. It had a hierarchical data model where keys could be nested. Removal of leaf key/node in a hierarchy would not automatically clean up the empty nodes that lead to the leaf node. Same problem was noted with zookeeper backend several years ago in #1964. The failing test case was introduced by that PR, but I think nobody tested with etcd2 server. It was even mentioned in the ticket "I am not sure if other backends do not share this bug (i.e. PSQL, MySQL, ETCd, S3, etc...)". Indeed, I believe this problem is relevant for etcd backend aswell. etcd3 introduced new flat data model and therefore nesting is implemented purely by key naming convention. Therefore the etcd3 backend implementation does not suffer from the problem of leaving empty nodes hanging around and the test case passes. |
Sure, that works. Thanks for the investigation and explanation re the test failure. I'm glad to hear it's not something we need to worry about given that we're dropping the v2 support. |
003e0ef
to
69cfc56
Compare
Hi @ncabatoff, I've now updated this PR by removing the failing etcd2 test case. I would like to request review for this PR in order to proceed with the client library uplift. Also, as discussed previously, I've posted new issue #12091 for removal of |
Hi @tsaarni, Sorry for the delay, we needed to see how this plays out on the enterprise side. The bad news is that it doesn't work there. We'll need to make changes to our gRPC-using code there before we can merge this. I'm afraid I can't commit to a time when that will happen, but we're eager to be able to upgrade these dependencies, and we don't want to get into a situation where a security issue forces our hand, so I don't think it will be a super long time. |
Hi @tsaarni, we've laid the groundwork on the enterprise side, so we're ready to accept this patch now. Could you pull in main and resolve conflicts please, then I'll merge it? |
69cfc56
to
f529962
Compare
@ncabatoff Cool! I've now rebased the PR with main. |
Looks like you have a conflict on go.sum. There's also a problem with the circleci tests not running, I'm looking into that. |
* Added etcd server container to run etcd3 tests automatically. * Removed etcd2 test case: it fails the backend tests but the failure is unrelated to the uplift. The etcd2 backend implementation does not remove empty nested nodes when removing leaf (see comments in hashicorp#11980). Signed-off-by: Tero Saarni <[email protected]>
Now the PR is up to date again. I see warning "First-time contributors need a maintainer to approve running workflows". |
Yeah, it was saying that before, and I approved it, but it didn't start running them. I did so again just now but it still seems stuck. |
I think I had this exact same experience with another project, also using circleci. Just a random thought but I wonder if manually approved workflows could get stuck when I've used force push? |
* Changelog Signed-off-by: Tero Saarni <[email protected]>
Out of curiosity, I added a new commit (just a period at the end of changelog entry - it can be squashed afterwards). |
Looks like it worked, good idea! And now you have a test failure (which looks like an easy fix) and a Solaris build failure, which I'm less confident about. I wonder if etcd still supports Solaris in recent versions? I know they used to... |
I will check the solaris build failure tomorrow. Regarding the other one:
I'm not connecting the dots yet, what is the root cause? |
Should be
Presumably relates to the version update of the atomic package. |
* Fixed TestInmemCluster_ConnectCluster after go.uber.org/atomic upgrade Signed-off-by: Tero Saarni <[email protected]>
Thanks, I should have spotted that! Regarding Solaris build, it is failing due to undefined socket option $ XC_ARCH=amd64 XC_OS=solaris XC_OSARCH=solaris/amd64 scripts/build.sh
==> Removing old directory...
==> Building...
Number of parallel builds: 15
--> solaris/amd64: github.com/hashicorp/vault
1 errors occurred:
--> solaris/amd64 error: exit status 1
Stderr: # go.etcd.io/etcd/client/pkg/v3/transport
../../go/pkg/mod/go.etcd.io/etcd/client/pkg/[email protected]/transport/sockopt_unix.go:14:54: undefined: unix.SO_REUSEPORT I did not see it mentioned anywhere that Solaris support would have been removed so I will check this. |
Submitted a question etcd-io/etcd#13298 |
Hi @tsaarni, Looks like the fix for etcd-io/etcd#13298 was merged. Would you mind refreshing this PR please? |
@ncabatoff It seems tricky to uplift while unreleased. I seem to end up tangled with dependencies between different etcd packages. I did not manage to get it updated even by forcing with |
@ncabatoff I got it sorted out but it is not pretty! First, I needed to figure out what the indirect dependencies are, then find out the pseudo-version identifier for the latest commit in Update: I should have realized that I can use released versions of other etcd modules and only |
… released versions of others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - we can tolerate a little bit of ugliness in go.mod for a while, it's far better than the current situation. Thanks so much for tackling this and for being patient with us.
This change updates the go client libraries for both etcd2 and etcd3.
Fixes #9915
Signed-off-by: Tero Saarni [email protected]